GH-50692: [C++][Parquet] treat negative null_count and distinct_count values as missing - #50694
GH-50692: [C++][Parquet] treat negative null_count and distinct_count values as missing#50694HuaHuaY wants to merge 1 commit into
Conversation
|
|
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
| const ColumnDescriptor* descr, | ||
| ::arrow::MemoryPool* pool) { | ||
| const auto& statistics = metadata.statistics; | ||
| const int64_t null_count = encoded_stats.has_null_count ? encoded_stats.null_count : 0; |
There was a problem hiding this comment.
Why can't we directly use metadata.statistics?
There was a problem hiding this comment.
Because the values which metadata.statistics contains are the negative values. I want to use the values modified in FromThrift.
| } | ||
| } | ||
| if (stats.__isset.null_count) { | ||
| if (stats.__isset.null_count && stats.null_count >= 0) { |
There was a problem hiding this comment.
This silently changes the public behavior that corrupted values are no longer visible to users from reading a file. I think we have a few options w.r.t. values in the stats and page indexes:
- Reject any corrupted values by throwing an exception (@etseidl has proposed this in another thread)
- Regard corrupted values as unset (this PR does).
- Preserve corrupted values but provide a
Validatefunction or a function to fix the corrupted values. - Do nothing so it is users' responsibility to check corrupted values and take action (ignore them or error out).
My understanding is that users have to validate these values before consumption anyway. In that case, an unset or corrupted value does not make too much difference. Users may still want to read the file even when there are corrupted stats (or even columns/row groups), e.g. recover corrupted/legacy files. So I think we shouldn't error out on invalid stats. Preserving corrupted values still bring some values like help inspecting the files so here might be the best place to fix this issue?
WDYT? @pitrou @mapleFU @emkornfield
There was a problem hiding this comment.
FYI:
- A brief discussion about option 1 and 2 Implement PARQUET-2249: Introduce IEEE 754 total order arrow-rs#9619 (comment)
- For option 4, we may need to fix
ParquetFileFragment::EvaluateStatisticsAsExpressionatcpp/src/arrow/dataset/file_parquet.cc. It usesbool may_have_null = !statistics.HasNullCount() || statistics.null_count() > 0;which regards negative count as zero.
There was a problem hiding this comment.
I see this was closed, but would just like to say that the situation here is a little different from arrow-rs. It seems arrow-cpp keeps the sign for null_count and nan_count, so returning the actual encoded value is probably the best thing to do. Let users decide what to make of that. arrow-rs, for unknown historical reasons, changes these fields to unsigned, so when a conversion to unsigned is not possible, the IMO most reasonable thing to do is return an error to let the user know something is wrong with the metadata. I don't think silently ignoring the error in that case is an option.
github-actions
Bot
added
awaiting committer review
|
Why do we care about this? The null_count could be incorrect in other ways (for example larger than num_values or inconsistent with the actual def levels). Writing a negative value is quite unlikely compared to the subtler ways of writing an incorrect value. The bottom line is that an incorrect statistic should not lead to a crash or UB, but it could lead to incorrect results when reading. There's nothing that we can reasonably do against that (the whole point of a statistic is to avoid reading all values, but verifying that a statistic is correct would require reading all values, defeating the point of a statistic). |
When I implement |
github-actions
Bot
added
awaiting changes
4 participants
Rationale for this change
To prevent negative
null_countanddistinct_countvalues from leading to incorrect statistics-based pruning, we had better to check for negative values.What changes are included in this PR?
Modify
ColumnIndex::Makeatcpp/src/parquet/page_index.ccandMakeTypedColumnStatsatcpp/src/parquet/metadata.cc. When readingnull_countanddistinct_countfrom a file, we no longer silently assume that the values are natural numbers.Are these changes tested?
Yes.
Are there any user-facing changes?
No.